[2.19] Add next-generation preview section to js-interop page #4528
Conversation
|
Thanks for adding this! I think it's what I had in mind for a future-proof interop, but I had a couple of comments. As for landing this, I can go either way. @kevmoo, do you think the users you had in mind who want to use future-proof interop will find it valuable to have this here versus pointing them to the issue? |
There was a problem hiding this comment.
Thanks for working on this! I have some minor suggestions, comments, and questions.
I definitely do think there is value in landing this since if the more future-proof "way" is already supported, this helps increase that awareness. In the best case this helps new JS-interop developers start by building this way rather than other ways while also helping existing JS-interop developers prepare or just know to keep an eye on this area.
This page is also linked to from the dart:js API docs as a destination to learn more about how developers should do things now. Sidenote: we should consider making the notice on that page more prominent.
|
Thanks @parlough , I'll incorporate the rest of your suggestions (the ones I'll have to update manually) once @srujzs responds with the details.
This is a good point and I think it changes the nature of the note saying this is only for people eager to prepare. I'll change the wording slightly to emphasize people who are just starting to implement js interop should do it this way. Edit: created dart-lang/sdk#51075 to track this point:
|
parlough
changed the title
|
Wooo... ok, that was a big change, thanks for the input everyone. I don't really expect anyone to review this right now, but it would be really nice to roll this in with the v2.19 branch, which is targeted for around 10am PST tomorrow. @sigmundch if you could verify the extra context I added based on your comments, that would be so helpful! @mit-mit @parlough , your feedback or approval is greatly appreciated too if you can squeeze that in! Thank you |
Co-authored-by: Parker Lougheed <[email protected]>
parlough
force-pushed
the
future-js-interop
branch
from
1840952
to
8362586
Compare
|
Visit the preview URL for this PR (updated for commit 79ea217): https://dart-dev--pr4528-future-js-interop-zfqketfc.web.app (expires Fri, 03 Feb 2023 22:17:21 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: d851bc446d3c4d7394c5406c6f07255afc7075f3 |
|
@sigmundch Actually not short on time anymore! We re-targeted this to
We can keep making changes, no deadline, and as far as I'm aware we don't freeze dart.dev ever anyway. So take your time! Thanks for your concern :) |
There was a problem hiding this comment.
Thank you so much @MaryaBelanger!
A few small suggestions below.
MaryaBelanger
added
review.copy
There was a problem hiding this comment.
@MaryaBelanger : Gave a light review. There's a spelling mistake in the copy and a few suggestions.
There was a problem hiding this comment.
Really great job on this and adjusting for everyone's comments. Sorry I didn't respond earlier, but somehow I accidentally(?) unsubscribed to this PR (oops) and didn't realize Sigmund reviewed it again already. I have a few minor suggestions.
Also consider trying to remove toc: false from the frontmatter(top) of this page now as well and seeing how that looks, since there is more content on the page now. When there were no headings the TOC wasn't useful so it was added to remove it.
I've pushed some minor formatting fixes as well. The main thing was that in a list we indent the content of a list entry to match the text on its first line (https://github.com/dart-lang/site-shared/blob/main/doc/markdown.md#do-properly-indent-markdown-lists).
src/web/js-interop.md
Outdated
| will help make interop even more idiomatic. | ||
|
|
||
| You can implement static interop using | ||
| the `package:js` annotation `@staticInterop`. |
There was a problem hiding this comment.
Optional: Consider linking this first @staticInterop to its API docs.
https://pub.dev/documentation/js/latest/js/staticInterop-constant.html
There was a problem hiding this comment.
Idk why I can't make that link work with {{site.pub-api}}/ syntax. I've tried a few ways but get a 404 every time
{{site.pub-api}}/staticInterop-constant
{{site.pub-api}}/js/staticInterop-constant
{{site.pub-api}}/js/js/staticInterop-constant
{{site.pub-api}}/js/latest/js/staticInterop-constant
There was a problem hiding this comment.
@MaryaBelanger Currently pub.dev and api.dart.dev aren't set up to handle a missing .html, I believe the last one should work:
{{site.pub-api}}/js/latest/js/staticInterop-constant.html
|
I can't tell if my comments have been accidentally duplicated or something - GitHub UI is confusing to me. Anyways, they've all been addressed so you can ignore them. This looks great, thanks for working on this! |
|
@srujzs Looks like the comments just weren't published until now for some reason, but I can see them now :) We appreciate all of your insights, comments, and answers! :D |
!!! Ugh, sorry about that, I guess I needed to click the "Start Review" button to publish. Luckily, Siggi's comments encapsulated all of mine. |
|
Added a couple minor edits just now. I'll wait a little while to merge just in case any final concerns are lingering, but I think this is done (@srujzs your comment conundrum confused me a little too 😅 I think everything you talked about is incorporated? But let me know if there's anything specific you want to reword/remove/link to/etc.) Thanks everyone! |
Staged: https://dart-dev--pr4528-future-js-interop-zfqketfc.web.app/web/js-interop
Fixes #4450